JS: Add backwards-compatible predicates to SocketIO#3262
Conversation
erik-krogh
left a comment
erik-krogh
left a comment
There was a problem hiding this comment.
Mostly fine. But you got a return-type wrong, and I think we can make the getAck predicates prettier.
(I found a spelling mistake: RecieveCallback needs to be ReceiveCallback, could you ninja-edit that into this PR?)
| result = getListener().getLastParameter() and | ||
| exists(result.getAnInvocation()) |
There was a problem hiding this comment.
| result = getListener().getLastParameter() and | |
| exists(result.getAnInvocation()) | |
| result.(ReceiveCallback).getReceiveNode() = this |
And then add a getReceiveNode in ReceiveCallback.
You could change the return-type to ReceiveCallback?
And similar in the other getAck predicates.
(SendNode::getAck returns SendCallback).
| SendNode getASender() { result.getAReceiver() = this } | ||
|
|
||
| /** Gets the acknowledgment callback, if any. */ | ||
| DataFlow::FunctionNode getAck() { |
There was a problem hiding this comment.
| DataFlow::FunctionNode getAck() { | |
| DataFlow::SourceNode getAck() { |
The two getAck methods on the ReceiveNode classes returned a DataFlow::SourceNode in the previous API.
(The getAck methods on SendNode did return DataFlow::FunctionNode in the previous API, so those are fine).
(If you change the return-types to SendCallback/ReceiveCallback as suggested in the below, then that will ensure the correct return-type).
| DataFlow::SourceNode getAReceivedItem() { result = getReceivedItem(_) } | ||
|
|
||
| /** Gets the acknowledgment callback, if any. */ | ||
| DataFlow::FunctionNode getAck() { |
There was a problem hiding this comment.
| DataFlow::FunctionNode getAck() { | |
| DataFlow::SourceNode getAck() { |
This is the second getAck that returned a DataFlow::SourceNode in the previous API.
| /** | ||
| * DEPRECATED. Always returns `this` as a `ServerObject` now represents the origin of a server. | ||
| * | ||
| * Instead of `getOrigin()` to get a server origin from a reference, use `ServerObject.ref()` to get references to a given server. | ||
| */ | ||
| deprecated DataFlow::SourceNode getOrigin() { result = this } |
There was a problem hiding this comment.
The QLDoc is not quite accurate.
In the previous API the ServerObject class was of type TServer, and there was a one-to-one relation between TServer and a SourceNode origin.
The getOrigin method extracted the SourceNode origin from the TServer. (One can say the method converted between the two representations).
It didn't have anything to do with references like .ref().
(References are handled by the ServerNode class, and I kept that part of the API).
I think you can just remove the second line of the QLDoc, and then its fine.
|
Thanks. PTAL |
|
Could you change the other 3 |
3 participants
Makes the API exposed by
SocketIObackwards compatible with clients from the 1.23 release.There are no DIL changes so an evaluation shouldn't be necessary.